Refactor image tree for API usage by saschagrunert · Pull Request #5093 · containers/podman

saschagrunert · 2020-02-05T12:13:07Z

rhatdan · 2020-02-05T19:49:23Z

vrothberg · 2020-02-07T13:58:18Z

Linter isn't happy yet. Will review now 👍

[+0036s] cmd/podman/tree.go:55:13: SA1006: printf-style function with dynamic first argument and no further arguments should use print-style function instead (staticcheck)
[+0036s] 	fmt.Printf(tree)

vrothberg

Looking really good, thanks @saschagrunert!

Can you also extend the swagger docs for the image endpoint? -> https://github.com/containers/libpod/blob/master/pkg/api/server/register_images.go#L581

The syntax is a bit clumsy but copy-pasting from the surrounding endpoints could help.

libpod/image/tree.go

saschagrunert · 2020-02-07T14:07:43Z

Linter isn't happy yet. Will review now

Thanks! Please keep in mind that the varlink API does not work yet :)

vrothberg · 2020-02-12T12:30:24Z

@baude @jwhonce PTAL

saschagrunert · 2020-02-14T08:57:27Z

Varlink API works now, ready for review.

vrothberg · 2020-02-14T12:22:14Z

Can you also extend the swagger docs for the image endpoint? -> https://github.com/containers/libpod/blob/master/pkg/api/server/register_images.go#L581

Still open.

vrothberg

Two nits.

I am not sure if we want add a Varlink endpoint, though. @baude @jwhonce PTAL.

libpod/image/tree.go

pkg/api/handlers/libpod/images.go

vrothberg · 2020-02-14T15:35:41Z

@saschagrunert, can you rebase? This will make local tests easier as we'd fetch podman system service.

pkg/api/server/register_images.go

pkg/api/handlers/libpod/images.go

saschagrunert · 2020-02-17T08:47:20Z

@saschagrunert, can you rebase? This will make local tests easier as we'd fetch podman system service.

Rebased on top of the latest master branch and addressed your review comments.

Signed-off-by: Sascha Grunert <sgrunert@suse.com>

saschagrunert · 2020-02-17T10:54:06Z

Docs seem to be green now as well :)

pkg/api/handlers/libpod/images.go

baude · 2020-02-17T15:17:35Z

it LGTM but i want @mheon to peek at this one

mheon · 2020-02-17T15:17:40Z

/approve

openshift-ci-robot · 2020-02-17T15:18:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, saschagrunert

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon · 2020-02-17T15:20:16Z

libpod/image/tree.go

@@ -0,0 +1,138 @@
+package image


Can we move this to somewhere in pkg/? It doesn't seem much like an API call, given it only returns a string.

I suggest to wait for @mtrmac's new package. We may be able to integrate it there.

By "wait" I mean we could merge it as is now and let Miloslav pick it up in his new package.

Hm. It's just libpod/image, not libpod itself, so sure

Definitely don’t wait for me, I’m still mostly in learning phase, and I can adjust for whatever happens here.

(Jumping into what I’m sure was a well-considered decision, I’d instinctively prefer to primarily have a well-typed API that returns the structure, and keep the text formatter in the CLI (allowing, at least in principle, a nice GUI to be built on top, for example). OTOH, I guess that exposing the complete tree structure over Varlink/whatever would be a lot of work and pain — and for no benefit at least now that the only consumer is the CLI.)

That's my big hold-up as well - I'd love to return a data structure that represents the tree, with a .String() that returns the output. I'm willing to defer that to followup work, though.

(Honestly, though, I'd be fine keeping the tree local-only, and exposing only a literal output string over the API. There's precedence for this with things like podman top)

(Or was it podman stats... I forget which)

mheon · 2020-02-17T16:19:25Z

LGTM

vrothberg

/lgtm

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 5, 2020

saschagrunert mentioned this pull request Feb 5, 2020

[Discussion] podman and cri-o shared codebase #2791

Closed

openshift-ci-robot added the size/L label Feb 5, 2020

openshift-ci-robot requested review from TomSweeneyRedHat and jwhonce February 5, 2020 12:13

vrothberg reviewed Feb 7, 2020

View reviewed changes

libpod/image/tree.go Outdated Show resolved Hide resolved

saschagrunert force-pushed the image-tree branch from beb8528 to 05471cd Compare February 14, 2020 08:49

saschagrunert changed the title ~~WIP: Refactor image tree for API usage~~ Refactor image tree for API usage Feb 14, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2020

saschagrunert force-pushed the image-tree branch from 05471cd to 0f5a902 Compare February 14, 2020 08:57

vrothberg reviewed Feb 14, 2020

View reviewed changes

libpod/image/tree.go Outdated Show resolved Hide resolved

pkg/api/handlers/libpod/images.go Outdated Show resolved Hide resolved

saschagrunert force-pushed the image-tree branch 4 times, most recently from 497ae0c to 3eda7ef Compare February 14, 2020 13:19

vrothberg reviewed Feb 14, 2020

View reviewed changes

pkg/api/server/register_images.go Outdated Show resolved Hide resolved

vrothberg reviewed Feb 14, 2020

View reviewed changes

pkg/api/handlers/libpod/images.go Outdated Show resolved Hide resolved

saschagrunert force-pushed the image-tree branch from 3eda7ef to 5342cad Compare February 17, 2020 08:46

saschagrunert force-pushed the image-tree branch from 5342cad to 5faeb8b Compare February 17, 2020 09:30

Refactor image tree for API usage

93358ef

Signed-off-by: Sascha Grunert <sgrunert@suse.com>

saschagrunert force-pushed the image-tree branch from 5faeb8b to 93358ef Compare February 17, 2020 10:12

TomSweeneyRedHat reviewed Feb 17, 2020

View reviewed changes

pkg/api/handlers/libpod/images.go Show resolved Hide resolved

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 17, 2020

mheon reviewed Feb 17, 2020

View reviewed changes

vrothberg reviewed Feb 17, 2020

View reviewed changes

openshift-ci-robot assigned vrothberg Feb 17, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2020

openshift-merge-robot merged commit 640b11f into containers:master Feb 17, 2020

This was referenced Feb 17, 2020

Fix handler and systemd activation errors #5158

Merged

Swagger: fix inconsistencies (try #2) #5231

Merged

saschagrunert deleted the image-tree branch February 17, 2020 16:25

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023

Conversation

saschagrunert commented Feb 5, 2020

Uh oh!

rhatdan commented Feb 5, 2020

Uh oh!

vrothberg commented Feb 7, 2020

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

saschagrunert commented Feb 7, 2020

Uh oh!

vrothberg commented Feb 12, 2020

Uh oh!

saschagrunert commented Feb 14, 2020

Uh oh!

vrothberg commented Feb 14, 2020

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vrothberg commented Feb 14, 2020

Uh oh!

Uh oh!

Uh oh!

saschagrunert commented Feb 17, 2020

Uh oh!

saschagrunert commented Feb 17, 2020

Uh oh!

Uh oh!

baude commented Feb 17, 2020

Uh oh!

mheon commented Feb 17, 2020

Uh oh!

openshift-ci-robot commented Feb 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Feb 17, 2020

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants